Skip to content

IGNITE-28896 SQL Calcite: Adjust fractional value handling in FETCH, OFFSET, and LIMIT - #13375

Merged
tkalkirill merged 17 commits into
masterfrom
ignite-28896
Aug 8, 2026
Merged

IGNITE-28896 SQL Calcite: Adjust fractional value handling in FETCH, OFFSET, and LIMIT#13375
tkalkirill merged 17 commits into
masterfrom
ignite-28896

Conversation

@tkalkirill

@tkalkirill tkalkirill commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

@tkalkirill tkalkirill changed the title IGNITE-28896 Wip IGNITE-28896 SQL Calcite: Support BigDecimal for FETCH, OFFSET and LIMIT Jul 15, 2026
@tkalkirill tkalkirill mentioned this pull request Jul 19, 2026
Merged
@zstan

zstan commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

So, it need to be merged somehow ? #13311
I don`t understand about rounding policy, this policy need to be applied globally for all rounding operations not only for limit\offset

@tkalkirill

tkalkirill commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@zstan BigDecimal is already in Calcite's main branch, so I think it makes sense to use that.
Regarding the rounding policy: the name FetchOffsetRoundingPolicy implies it doesn't cover all rounding, but specifically OFFSET and FETCH, and the documentation describes it that way as well.

zstan
zstan reviewed Aug 6, 2026
View reviewed changes
Comment thread .../apache/ignite/internal/processors/query/calcite/integration/LimitOffsetIntegrationTest.java Outdated
Comment thread ...ain/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java
zstan
zstan reviewed Aug 7, 2026
View reviewed changes
Comment thread ...test/java/org/apache/ignite/internal/processors/query/calcite/integration/DataTypesTest.java Outdated
Comment thread .../apache/ignite/internal/processors/query/calcite/integration/LimitOffsetIntegrationTest.java Outdated
@zstan

zstan commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@tkalkirill plz fix minor changes, overall looks good, call me to review after final fix plz

@tkalkirill
tkalkirill requested a review from zstan August 7, 2026 13:31
zstan
zstan reviewed Aug 7, 2026
View reviewed changes
Comment thread .../apache/ignite/internal/processors/query/calcite/integration/LimitOffsetIntegrationTest.java

/** */
@Test
public void testFractionalLimitOffset() throws Exception {

@zstan zstan Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have the same tests in script ones: limit.test do you think we also need it here ?

@tkalkirill tkalkirill Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep it.

zstan
zstan approved these changes Aug 7, 2026
View reviewed changes

@zstan zstan left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good if TC is ok, also i think that we have duplicated tests functionality through Integration and script tests, don`t forget to change initial issue description cause now it`s not about decimal but about rounding as i can see

@tkalkirill tkalkirill changed the title IGNITE-28896 SQL Calcite: Support BigDecimal for FETCH, OFFSET and LIMIT IGNITE-28896 SQL Calcite: Adjust fractional value handling in FETCH, OFFSET, and LIMIT Aug 8, 2026
@tkalkirill
tkalkirill merged commit 2a37fcb into master Aug 8, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zstan zstan zstan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tkalkirill @zstan